Skip to content

WIP: GoDaddy Contributions - #47

Closed
shelbys wants to merge 4 commits into
strongloop:masterfrom
shelbys:master
Closed

WIP: GoDaddy Contributions#47
shelbys wants to merge 4 commits into
strongloop:masterfrom
shelbys:master

Conversation

@shelbys

@shelbys shelbys commented Sep 30, 2014

Copy link
Copy Markdown
Contributor

Added support for merging nested and complex data structures in configuration files in order to better support different paths across multiple environments

@slnode

slnode commented Sep 30, 2014

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

* upstream/master:
  Update contribution guidelines
  test: ensure sandbox dir is present
  test: add `global.navigator` for browser tests
  test: increase timeout for browserify
  index: fix jshint error

Conflicts:
	index.js
@bajtos

bajtos commented Oct 8, 2014

Copy link
Copy Markdown
Member

Hi @shelbys, thank you for the contribution. Few things to improve the next time you submit a pull request (I addressed them myself in #51 and #52 for this time):

  • We use 50/72 format for commit messages - wrap the lines at 50/72 chars, use imperative format ('change foo' instead of 'changed foo').
  • Create a new feature branch for your changes, that way you can keep the changes isolated from other things you are doing in your master branch and also it makes it safe(r) to rewrite git history.
  • When you want to bring in upstream changes to you feature branch, run git fetch upstream && git rebase upstream/master. That way your commits are replayed on top of the current master, there is no merge commit added and the git history remains linear.
  • Changes in non-code files like .gitignore should be made in a standalone commit.

In order to land your patch, I need you to sign our CLA first. Just follow the instructions here: https://cla.strongloop.com/agreements/strongloop/loopback-boot

Note that will need the same for other repositories where you contributed pull requests too:

@shelbys

shelbys commented Oct 8, 2014

Copy link
Copy Markdown
Contributor Author

@bajtos - We use node_modules/.bin/mocha to make it obvious that global isn't expected, and to easily copy to command-line for running single and other experiments

@shelbys

shelbys commented Oct 8, 2014

Copy link
Copy Markdown
Contributor Author

@bajtos - Regarding CLA, GoDaddy already has a master agreement in place

@bajtos bajtos closed this in #52 Oct 9, 2014
@bajtos bajtos removed the #wip label Oct 9, 2014
@bajtos bajtos reopened this Oct 9, 2014
@bajtos bajtos added the #review label Oct 9, 2014
@slnode

slnode commented Oct 9, 2014

Copy link
Copy Markdown

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@rmg

rmg commented Oct 9, 2014

Copy link
Copy Markdown
Member

@shelbys The flow is based on the signer's GitHub credentials so we can't manually add your account. It would help us out a lot if you could click through the signing process (it's quick, I promise!) at https://cla.strongloop.com/agreements/strongloop/loopback-boot so that we don't get in the habit of ignoring CLA warnings on our PRs.

@shelbys

shelbys commented Oct 9, 2014

Copy link
Copy Markdown
Contributor Author

@rmg - Done

@altsang

altsang commented Oct 10, 2014

Copy link
Copy Markdown

thx @shelbys ! it's my fault, I told @rmg not to worry about faking out the system and just asking to have it clicked through as the path of least resistance, thx again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants